-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test coverage for entrypoint #25
base: main
Are you sure you want to change the base?
Conversation
All necessary updates are done to make it functional. Steps to try testsInstall the following packages, for example by adding to the Dockerfile:
Then add the content of the
You may have some warning, sometime they come, sometime they disappear depending on your configuration. Read/write error of a cache, I didn't investigate further for now. |
I'm testing with this Dockerfile, created as
That works. |
Maybe some issue because of |
I enabled a hook for setgid/setuid. I'm thinking about integration with GA on pull requests. Do you think we should create a subdirectory (I was thinking of To integrate with GA, I was thinking about using a Dockerfile for tests, or to use |
I would recommend to create a
|
Ideally, the unit tests for the
What do you want to cache? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review.
Overall looks really good, but some things should be moved from unit to integration because they don't test atomic functions, but end results external to entrypoint, and some uses of os
probably will prevent portability of the tests.
I think if we fix these, this also becomes platform independent, so that a windows dev can run these tests natively too.
tests/test_dockerfile.py
Outdated
hook.test(test_args, test_environ, result_args, result_environ) | ||
assert hook.result == hook.reference, hook.error_msg() | ||
|
||
#Test datadir metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be moved to an integration test too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it, keep record of the test in #33 to convert it in CI tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a mistake in this comment, I didn't remove this test, it's the previous one. It verifies that datadir is properly created by the entrypoint + chown, it manages the creation of the directory. Don't you think I should keep it here ?
tests/test_dockerfile.py
Outdated
hook.test(test_args, test_env, result_args, result_env) | ||
assert hook.result == hook.reference, hook.error_msg() | ||
|
||
def test_equal_argv_and_env(hook): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: why is this designed like this?
I'm not entirely sure about the feature. I'd like to know why we do that because I think it can create error situations: if I do -e PAYTXFEE=1
and add -paytxfee=0.1
, which one will be used by dogecoind and how do I know that as an operator. Shouldn't we protect the operator against that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had no idea what should I do in this case, just had the idea to test it. The user still can pass multiple times the same arguments, Dogecoin Core accept it, with or without this environment.
I don't know what happen internally, but the following start the node:
./bin/dogecoind -maxtxfee=1 -maxtxfee=2
Idk if it should be handled here, or maybe improve dogecoin/dogecoin#1313 to let Dogecoin Core deal with it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dogecoind
takes the last entry. Which in this case means the last arg overwrites the env value.
So if we're at this point not protecting the user (I'm fine with this, just need to make a decision on it) perhaps it makes sense to do a different value assignment for an env and arg of the same type and then test if the resulting order is correct. A rewrite in the logic could easily change this and mess things up, so this would be really good to test.
We can let dogecoin/dogecoin#1313 handle this better for users regardless of how they deploy, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sense to do a different value assignment for an env and arg of the same type and then test if the resulting order is correct
You're speaking about the second assertion of this test ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be great to protect this behavior, but yeah maybe it's more appropriate to handle double configuration inside the Core, make sense to me. Needed just for args for example, and would duplicate verification if it's done here.
tests/test_dockerfile.py
Outdated
|
||
def abs_path(executable): | ||
"""Build manually (expected) executable absolute path""" | ||
return os.path.join(EXECUTABLES_FOLDER, executable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would add a \
when ran on windows, I think that's not what we're looking for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on what we want to do, maybe. The answer may differ if we can run those tests from outside a container.
tests/test_dockerfile.py
Outdated
test_environ = { | ||
"DATADIR" : "/dogecoin/.dogecoin", | ||
"USER" : "dogecoin", | ||
"PATH" : os.environ["PATH"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about os.environ
- can we do this with a fixture instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem. If shared with the host, yes it may be an issue, if it's shared by the container it shouldn't be a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me ask it differently: what benefit does this give over a static fixture? Do we need this to be a working path? If yes, for what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entrypoint use PATH
to resolve executable location in shutil.which
:
docker/1.14.5/bullseye/entrypoint.py
Line 25 in 059cf4a
executable_path = shutil.which(executable) |
Entrypoint need at least /usr/local/bin
in PATH
to work properly [in a container].
The issue I see to do it outside a container is that entrypoint need for now the man pages to retrieve executables options, and will even launch an executable when #28 will be implemented. That's for me the main problem, testable from outside but the host must provide man pages replaced by executables in a near future. Maybe the easiest way is to run it inside a container. About caching, with my download bandwidth:
Annoying between tests if it always needs to be reinstalled. P.S.: Current tests do not perform unit test of each entrypoint function, but an overall test of the entrypoint behavior through the main. I don't get all subtleties about test naming convention and I'm not able to categorize it as you do, just to be sure you are clear with that. |
Good point, that is a dependency outside of the entrypoint, so in this case we'll want to mock that - basically do something similar to how you mock execution right now. It makes the test of entrypoint.py more generic and not dependent on the Dogecoin Core version used.
Yes. That's why unit tests are better to be executed on the host where you do development. pip3 is also pre-installed on the GA image together with python.
I don't want to go all purist on this, because I think it serves no purpose to do so at this point. If something is covered in unit tests, it's covered. If we miss coverage of some important cases, we can add the test for it. I think we shouldn't worry too much about whether it's called atomically or not right now, maybe something to work on later. In terms of things being tested:
That covers all components, except dependencies. |
To mock man pages/executables, you would like to generate a fake help menu to parse with a limited list of option ? It's an important component to detect which environment variables are valid or not, I'm not sure how we could hook it to reproduce the behavior. For example, it may be something to test, try one environment variable with 2 different executable with specific argument from each, where the environment variable is only used by one executable, to see if entrypoint doesn't confuse between different help menus. |
let's look inside
As it is right now, that would need to be mocked completely by a function that just returns a list of options for a given executable. Possible, but that excludes that entire function from testing. It could be an idea to put the man page reading in its own function and then mock that to just return a couple of fixtures to read instead. Then we can test everything else in that function, including test if the function itself does everything we want it to do and doesn't mess up. |
Created #34 to use executables help menu instead of man pages to get options. To create hook on top of it. |
Still a bit messy, but it's functional now and can run from the host. I had to add some extra hook and change some behavior. Not friendly to test while it's not in his own folder. Do we call its folder |
tests/entrypoint_hook.py
Outdated
#Disable setgid & setuid behavior | ||
os.setgid = lambda _ : None | ||
os.setuid = lambda _ : None | ||
shutil.which = lambda executable : f"/usr/local/bin/{executable}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move line and/or update doc
tests/test_dockerfile.py
Outdated
EXECUTABLES_FOLDER = "/usr/local/bin/" | ||
|
||
#Set tests user and directories | ||
TEST_USER = os.environ.get("TEST_USER", None) | ||
if TEST_USER is None: | ||
TEST_USER = "dogecoin" | ||
|
||
TEST_DIRECTORY = tempfile.TemporaryDirectory() | ||
TEST_DATADIR = TEST_DIRECTORY.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some possibilities do define tests variables in conftest.py
.
so following @patricklodder directions to run these tests as of a day or so ago:
i moved your test files from tests/ to unit/ and created a basic Dockerfile.test identical to @patricklodder's above:
from the outputted image hash after building 1.14.5/bullseye/Dockerfile:
which resulted in:
🎆 🥳 woohoo! so with that said it's almost there. am wondering how we're going to 'plug' this in? will it be ran as part of CI? not sure the grand vision but we could potentially use ONBUILD within our existing Dockerfile (at the bottom) to pass the image hash as it executes after our main Dockerfile is built and run tests and then delete/clean up after execution? outside of that i'd say a squash + rebase is needed and then ACK from me. 👍 i also pushed this as reference in case needed but in no way is 'the correct' way, just how i understood testing this PR. |
I will add some fixes about global variables, and try to enable only hooks partially when tests are launched inside containers, to be closer to the real behavior. |
Recommend against that, it will make tests non-deterministic. I can propose an integration test for the chmod results for you if you want. |
I will do something in a separate commit, it looks interesting to me to just add a way to disable some hook of Having 2 mode may enable us to tests some functions when inside the container, like |
Not inside unit tests, I coded us an integration test framework for that. |
Some fun with pylint:
Pylint does have some trouble to work with pytest, it raises some false positive. There is some alternative python linter for pytest like pytest-pylint (maybe not well maintained...), I saw a Stack Overflow comment suggesting the following:
The solution I chose for now is to hide those errors, thought it could be acceptable here, for these specifics errors:
Pylint need some adjustment in any case, CI tests crashes without pytest installed, see GA results. |
Ideally, our pylint check should not lint pytest, but limit scope only our code. I'm trying to figure out what happens if we inline ignore the pytest import and if that still yields meaningful results, or, import pytest with the linting run and then exclude that module's errors rather than the ones inherited under the module we wrote... but ignores are needed. Once we introduce ignores, it is probably prudent to explicitly list the allowed linter ignores and document why they are allowed. |
Yes, more tests to automate :) Ready for a new round of review. Recent changes allow tests to run both from host and inside a container. I implemented 2 modes for tests (introduced by 174640a), it detects if it runs inside a container to avoid the hook of May enable some specifics tests to runs inside a container which is not permitted by, for example verify the behavior of entrypoint for non-dogecoin commands. Pass pylint tests with exception from PS: Some files may need a renaming (any suggestions ?). |
awesome! will dig into this later this afternoon when i can checkout your changes and address your comments/concerns properly, have to run some errands rq. ❤️ |
New changes related to previous comments. For now, functionally I think it's good, I organized the tree structure (pylintrc to move away probably), dividing all tests in separate files, creating a hook directory for tools needed. I also enabled some default environment variables (PATH & USER) in EntrypointHook to avoid repetition in tests. It gives the following structure:
Possibly not far from being ready :) Recap on how to launch testsIt can run from hosts or from container, with fewer hooks to enable ( From hostMove into the From a containerBuild image from # Inside the host
docker run -it -v $(pwd)/unit:/tests [image-tag] bash
# From the container bash
apt update && apt install -y python3-pip
python3 -m pip install pytest
# Then move in mounted dir and run tests
cd /tests && pytest |
Integration to GitHub Action added, you can see directly results from PR CI tests :)
Squash to clean up the PR. Need to configure linter to handle issues, to do in a separate PR prior to it. EDIT: pylitnrc removed in favor of #52 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, much of this is really good ❤️
I have 2 blockers:
- Please get rid of containerization detection. Any test goals that are achieved with this MUST be reflected in integration tests.
- The sort of resulting arguments in a command must go, it prevents testing order of argument parsing, which matters.
I removed the container mode. About sorting, I removed it. But I wasn't sure actually how it was managed by Completing #46, the current behavior is:
As you can see in docker/1.14.5/bullseye/entrypoint.py Line 109 in 9dcc169
It finds all options through help menu, iterate through those options and try to find if it's available in the environment. I fixed tests in 1105189 to reflect this order. Merry Christmas 🎅 |
Add test coverage for Dockerfile and the
docker-entrypoint.py
script.Run a variety of tests against entrypoint to verify the command which should have been launched by execve. Verifying available files of the image, and the data directory creation.
I hope it can be a good base to test the image, something with meaningful log & understandable enough to be extendable + debuggable.
Need some modification to work appropriately, I will PR it right after:
docker-entrypoint.py
toentrypoint.py
(for import) Create main function in entrypoint + rename entrypoint #26Launch tests
For now, it's not plugged appropriately with the code and will crash because there is caught error to fix, but for when it will be ok:
Install pip & python packages for the container
apt update && apt install -y python3-pip python3 -m pip install pytest pytest-testinfra
Add tests files in the container & run
pytest
in their directory.Let me know what you think about these tests. And if you have any suggestion of things to tests !